Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.62%. Comparing base (755ba9c) to head (b5a3d89).
Report is 1 commits behind head on master.

@@            Coverage Diff             @@
##           master     #860      +/-   ##
==========================================
+ Coverage   95.57%   95.62%   +0.05%     
==========================================
  Files          30       30              
  Lines        4924     4981      +57     
  Branches      447      464      +17     
==========================================
+ Hits         4706     4763      +57     
  Misses        192      192              
  Partials       26       26              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mjpieters I started looking into using Hypothesis during PyCon (the maintainer helped me start).
Does this look like a yarl bug to you? https://github.com/aio-libs/yarl/actions/runs/4813151119/jobs/8569304512?pr=860#step:7:1489

@mjpieters I started looking into using Hypothesis during PyCon (the maintainer helped me start). Does this look like a yarl bug to you? https://github.com/aio-libs/yarl/actions/runs/4813151119/jobs/8569304512?pr=860#step:7:1489

I was going to look into that since I found this PR! It does look like %30 is not being decoded properly, so that does look an awful lot like a bug.

Actually, no this is not a bug in Yarl; it is a bug in your Hypothesis tests. unsafe contains '0':

test_quote_unquote_parameter(
    quoter=yarl._quoting_c._Quoter,
    unquoter=yarl._quoting_c._Unquoter,
    text_input='0',
    safe='',
    unsafe='0',   # <--- 0 is unsafe
    protected='',
    qs=False,
    requote=False,
)

With 0 unsafe, the unquoter must return %30.

Either not tell Hypothesis to provide input to unsafe, or account for characters in unsafe when comparing input and output.

@mjpieters yeah, I never used these and wasn't sure about the semantics which is why I asked you. My objective is to make the foundation for adding more Hypothesis testing in the future and so the contributors could look at the examples...

@mjpieters how about https://github.com/aio-libs/yarl/actions/runs/5206601078/jobs/9393336950?pr=860#step:7:1507 then?

Got it working with a basic implementation. I figure we can always expand it later.

not sure why it fails on 3.8 and 3.9 linux/mac only

@bdraco the tests don't fail but overall pytest session does, due to low coverage it's probably hitting fallback code branches:

FAIL Required test coverage of 98.95% not reached. Total coverage: 98.84%

(https://github.com/aio-libs/yarl/actions/runs/11194802544/job/31121813637?pr=860#step:10:2589)

We should just update .coveragerc to lower the expectations since those are for a single coverage run which will never hit all lines for a single test run. We rely on Codecov's combined metrics instead.

Also, could you use the “rebase” button on the UI for my branches, instead of producing foxtrot merges?

@bdraco the tests don't fail but overall pytest session does, due to low coverage it's probably hitting fallback code branches:

FAIL Required test coverage of 98.95% not reached. Total coverage: 98.84%

(https://github.com/aio-libs/yarl/actions/runs/11194802544/job/31121813637?pr=860#step:10:2589)

We should just update .coveragerc to lower the expectations since those are for a single coverage run which will never hit all lines for a single test run. We rely on Codecov's combined metrics instead.

👍

Also, could you use the “rebase” button on the UI for my branches, instead of producing foxtrot merges?

I will keep that preference in mind for the future.

Also, I only show an update branch button and no rebase button in the UI, but happy to rebase manually instead.

Got it working with a basic implementation. I figure we can always expand it later.

That sounds reasonable. It'd be nice to have own strategy exposed under yarl.testing so that the end-users could use that strategy in their tests.

Also, I only show an update branch button and no rebase button in the UI, but happy to rebase manually instead.

@bdraco it's the same button, but it's a bit unobvious. There's an arrow with a drop-down on the update button (on its right side) that lets you select the mode (it won't let you select rebase if there are conflicts, though — in that case it'd require doing it locally).

Also, I only show an update branch button and no rebase button in the UI, but happy to rebase manually instead.

@bdraco it's the same button, but it's a bit unobvious. There's an arrow with a drop-down on the update button (on its right side) that lets you select the mode (it won't let you select rebase if there are conflicts, though — in that case it'd require doing it locally).

O wow. I apparently have been missing that for years 🤦

Its unfortunate that GitHub doesn't have a configuration option to order it based on preference

https://github.com/orgs/community/discussions/12032

https://github.com/motlin/forbid-merge-commits-action might be an option

motlin/forbid-merge-commits-action might be an option

@bdraco so my strategy is tailored to individual PRs. If a contributor maintains their commits in the PR branch well, I prefer merging such PRs using the natural merge. And that's what I like to do in my PRs, as I control them. When I experiment, I may add some silly WIP commits but then, I always clean up the history before merging. When somebody has a mess, including but not limited to foxtrot merges in their PR, only then I consider using the “squash merge” option.
But since I don't like unnatural merge methods, I work with my own branches in a more self-aware way. So my ask is about my branches, not arbitrary contributors'. This is also because we can't expect random people on the internet to know how to use Git like a pro, which is why I wouldn't use automation to block such PRs and create barriers for the contributors (even though, my instinct is usually to tell people to maintain their commits well).

Thanks for clarifying. Apologizes if I overstepped.

No problem! After all, I never explained this explicitly, nor did I deselect the checkbox to allow others to change the branch in my work. All's good :)